tests/fuzz/fuzz-open_channel: fix bad local variable access after longjmp - #9165
Conversation
Andezion
left a comment
Andezion
left a comment
There was a problem hiding this comment.
The fix correctly identifies and resolves a real UB bug per the C standards setjmp/longjmp rules!
| */ | ||
| const tal_t *run_ctx = tal(NULL, tal_t); | ||
|
|
||
| if (setjmp(fuzz_env) != 0) |
There was a problem hiding this comment.
What do you think, did tests/fuzz/fuzz-handle_onion_message.c (also using jmp_buf fuzz_env and setjmp(fuzz_env) at line 78) has the same ordering issue?
There was a problem hiding this comment.
The use of setjmp/longjmp in tests/fuzz/fuzz-handle_onion_message.c shouldn't cause a crash since the only local variable (daemon) accessed after the longjmp is initialized (to NULL) before the setjmp. However, the call to tal_free(daemon->master) may or may not happen as intended, depending on whether the compiler emits instructions to reload the register holding the value of daemon after the longjmp. If the register is reloaded (from the stack), then it may hold the non-null address to which daemon was set from the return value of new_daemon() (depending on whether the compiler emitted instructions after the call to new_daemon() to flush the new value of daemon back onto the stack), and tal_free(daemon->master) will be called if this has occurred. On the other hand, if the register is not reloaded, then it will still hold the value NULL (the value to which daemon was initialized before the setjmp call), and tal_free(daemon->master) will not be called. I would argue that it's not a good idea to have control flow vary depending upon compiler optimizations. You can prevent the compiler from caching the daemon local variable in a register by declaring it (i.e., the pointer itself, not the pointed-to object) volatile, but I generally wouldn't recommend that, as volatile is detrimental to compiler optimizations. A nicer fix would be to insert a second call to setjmp after daemon is set to the return value from new_daemon(). That would ensure that the code at the cleanup label will always see the latest value of daemon, even in the case that cleanup is reached via a longjmp.
…gjmp You can't access a local variable from a point before it was initialized and expect it to have the initialized value. Move the setjmp() call to after run_ctx is initialized so that the tal_free() call at cleanup will see the correct address and not crash. Fixes: ElementsProject#9131 Changelog-None
03e5441 to
e74c588
Compare
4 participants
You can't access a local variable from a point before it was initialized and expect it to have the initialized value. Move the
setjmp()call to afterrun_ctxis initialized so that thetal_free()call atcleanupwill see the correct address and not crash.Fixes: #9131
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgradeN/A